refactor map page to TSX and split into focused modules#47
Conversation
- Extract map concerns into hooks: useMapListingUrl (URL sync + optimistic pin selection to fix tap flicker), useListingsInView (debounced, cancellable, padded viewport fetch that preserves prior pins), useMapCenter (centralised fly-to rules), useIpInitialLocation, and useMapDrawerScroll. - Split MapPageClient into MapPageClient + MapListingDrawer and MapImmersive into MapImmersive + MapPinLayer; move shared types and helpers into src/utils/mapUtils.ts. - Convert MapPageClient, MapImmersive, MapPin, MapSearch, and MapSidebar (plus their barrels) from JSX to TSX with typed props. - Strip extraneous console logs and add i18n strings for the return-to- listing button and the pin-loading chip. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Pull request overview
Refactors the /map page into a typed src/features/map/ feature folder, consolidating map/listing URL + drawer + centering logic while improving pin-selection UX and viewport-driven pin fetching.
Changes:
- Introduces shared
Listing/marker/selection types and centralised map utilities (bounds padding, snap points, sidebar width, fly durations). - Rebuilds the map page client into focused hooks/components (
useMapListingUrl,useListingsInView,useMapCenter,useMapDrawerState,MapView,MapListingDrawerPanel). - Converts key components to TSX and removes legacy map components/entrypoints; adds i18n strings for “return to listing” and “loading pins”.
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/listing.ts | Adds shared listing/marker/selection types and type guards for map + listing UI. |
| src/features/map/lib/mapUtils.ts | Introduces shared constants and helpers (bounds padding, snap points, coordinate guards). |
| src/features/map/index.ts | Feature barrel export for map page client entry. |
| src/features/map/hooks/useMapListingUrl.ts | Centralizes URL ↔ selected listing sync and optimistic pin selection on tap. |
| src/features/map/hooks/useMapDrawerState.ts | Consolidates drawer snap/header/html-class state and scroll listener setup. |
| src/features/map/hooks/useMapCenter.ts | Centralizes programmatic map motion rules (URL selection, return-to-listing, search pick). |
| src/features/map/hooks/useListingsInView.ts | Adds debounced, stale-response-safe viewport pin fetching with padded bounds. |
| src/features/map/hooks/useIpInitialLocation.ts | Adds guarded MapTiler client init and IP-based initial centering. |
| src/features/map/components/MapView.tsx | New TSX map view (style constant, pin layer, search, fetch-on-move, return button, loading chip). |
| src/features/map/components/MapSidebar.tsx | Updates sidebar to TS-friendly props/types and reuses shared sidebar width constant. |
| src/features/map/components/MapSearch.tsx | New feature-local MapTiler geocoding control wrapper. |
| src/features/map/components/MapPinLayer.tsx | Extracts marker rendering/selection + drawer trigger wiring into a dedicated layer component. |
| src/features/map/components/MapPageClient.tsx | New orchestrator for map view + drawer + sidebar + hooks. |
| src/features/map/components/MapListingDrawerPanel.tsx | Extracts drawer UI/panel into focused TSX component. |
| src/components/MapSidebar/index.js | Removes legacy map sidebar re-export entrypoint. |
| src/components/MapSearch/index.js | Removes legacy map search re-export entrypoint. |
| src/components/MapSearch/MapSearch.jsx | Deletes legacy MapSearch implementation (moved into feature). |
| src/components/MapPin/index.ts | Adds TS barrel export for MapPin. |
| src/components/MapPin/MapPin.tsx | Updates MapPin typing and cleans up styling logic. |
| src/components/MapPageClient/index.js | Removes legacy map page client re-export entrypoint. |
| src/components/MapPageClient/MapPageClient.jsx | Deletes legacy monolithic map page client implementation. |
| src/components/MapImmersive/index.js | Removes legacy MapImmersive re-export entrypoint. |
| src/components/MapImmersive/MapImmersive.jsx | Deletes legacy MapImmersive implementation (replaced by MapView + hooks). |
| src/components/ListingRead/index.ts | Adds TS barrel export for ListingRead. |
| src/components/ListingRead/ListingRead.tsx | Converts ListingRead to TSX and localizes chat drawer state. |
| src/components/ListingChatDrawer/index.ts | Adds TS barrel export for ListingChatDrawer. |
| src/components/ListingChatDrawer/ListingChatDrawer.tsx | Converts ListingChatDrawer to TSX and formalizes props/types. |
| src/app/actions.ts | Removes a stray console.log from fetchListingsInView. |
| src/app/(core)/(interact)/(stretched)/map/page.tsx | Converts map route to TSX with typed metadata + feature import. |
| messages/es.json | Adds Map.returnToListing and Map.loadingPins strings. |
| messages/en.json | Adds Map.returnToListing and Map.loadingPins strings. |
| messages/de.json | Adds Map.returnToListing and Map.loadingPins strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {hasInitialPosition && ( | ||
| <> | ||
| <Map | ||
| ref={mapRef} | ||
| attributionControl={false} | ||
| mapStyle={MAP_STYLE} | ||
| renderWorldCopies={true} | ||
| initialViewState={resolveInitialViewState( | ||
| selectedListing, | ||
| initialCoordinates | ||
| )} | ||
| onMoveEnd={handleMoveEnd} | ||
| onLoad={handleLoad} | ||
| onClick={handleMapClickInternal} | ||
| > | ||
| <GeolocateControl showUserLocation={true} /> | ||
| <NavigationControl showZoom={true} showCompass={false} /> | ||
|
|
||
| <AttributionControl | ||
| compact={true} | ||
| style={ | ||
| !isDesktop | ||
| ? attributionControlMobileStyle | ||
| : attributionControlDesktopStyle | ||
| } | ||
| /> | ||
|
|
||
| <MapPinLayer | ||
| listings={listings} | ||
| selectedListingId={selectedListingId} | ||
| DrawerTrigger={DrawerTrigger} | ||
| onMarkerClick={onMarkerClick} | ||
| /> | ||
| </Map> | ||
|
|
||
| <MapSearch | ||
| onPick={handleSearchPick} | ||
| countryCode={countryCode} | ||
| style={searchStyle} | ||
| /> | ||
|
|
||
| {isFetching && <LoadingChip>{t("loadingPins")}</LoadingChip>} | ||
|
|
||
| {showReturnButton && ( | ||
| <ReturnToListingButton | ||
| onClick={flyToSelected} | ||
| variant="secondary" | ||
| size="small" | ||
| width="contained" | ||
| > | ||
| {t("returnToListing")} | ||
| </ReturnToListingButton> | ||
| )} | ||
| </> | ||
| )} |
There was a problem hiding this comment.
hasInitialPosition prevents the <Map> from rendering until either initialCoordinates resolves or a selected listing has valid coordinates. On first load with no listing param, initialCoordinates starts as null (and can remain null on timeout/error), which means the map never renders and contradicts the comment that MapView falls back to DEFAULT_COORDINATES.
| {hasInitialPosition && ( | |
| <> | |
| <Map | |
| ref={mapRef} | |
| attributionControl={false} | |
| mapStyle={MAP_STYLE} | |
| renderWorldCopies={true} | |
| initialViewState={resolveInitialViewState( | |
| selectedListing, | |
| initialCoordinates | |
| )} | |
| onMoveEnd={handleMoveEnd} | |
| onLoad={handleLoad} | |
| onClick={handleMapClickInternal} | |
| > | |
| <GeolocateControl showUserLocation={true} /> | |
| <NavigationControl showZoom={true} showCompass={false} /> | |
| <AttributionControl | |
| compact={true} | |
| style={ | |
| !isDesktop | |
| ? attributionControlMobileStyle | |
| : attributionControlDesktopStyle | |
| } | |
| /> | |
| <MapPinLayer | |
| listings={listings} | |
| selectedListingId={selectedListingId} | |
| DrawerTrigger={DrawerTrigger} | |
| onMarkerClick={onMarkerClick} | |
| /> | |
| </Map> | |
| <MapSearch | |
| onPick={handleSearchPick} | |
| countryCode={countryCode} | |
| style={searchStyle} | |
| /> | |
| {isFetching && <LoadingChip>{t("loadingPins")}</LoadingChip>} | |
| {showReturnButton && ( | |
| <ReturnToListingButton | |
| onClick={flyToSelected} | |
| variant="secondary" | |
| size="small" | |
| width="contained" | |
| > | |
| {t("returnToListing")} | |
| </ReturnToListingButton> | |
| )} | |
| </> | |
| )} | |
| <> | |
| <Map | |
| ref={mapRef} | |
| attributionControl={false} | |
| mapStyle={MAP_STYLE} | |
| renderWorldCopies={true} | |
| initialViewState={resolveInitialViewState( | |
| selectedListing, | |
| initialCoordinates | |
| )} | |
| onMoveEnd={handleMoveEnd} | |
| onLoad={handleLoad} | |
| onClick={handleMapClickInternal} | |
| > | |
| <GeolocateControl showUserLocation={true} /> | |
| <NavigationControl showZoom={true} showCompass={false} /> | |
| <AttributionControl | |
| compact={true} | |
| style={ | |
| !isDesktop | |
| ? attributionControlMobileStyle | |
| : attributionControlDesktopStyle | |
| } | |
| /> | |
| <MapPinLayer | |
| listings={listings} | |
| selectedListingId={selectedListingId} | |
| DrawerTrigger={DrawerTrigger} | |
| onMarkerClick={onMarkerClick} | |
| /> | |
| </Map> | |
| <MapSearch | |
| onPick={handleSearchPick} | |
| countryCode={countryCode} | |
| style={searchStyle} | |
| /> | |
| {isFetching && <LoadingChip>{t("loadingPins")}</LoadingChip>} | |
| {showReturnButton && ( | |
| <ReturnToListingButton | |
| onClick={flyToSelected} | |
| variant="secondary" | |
| size="small" | |
| width="contained" | |
| > | |
| {t("returnToListing")} | |
| </ReturnToListingButton> | |
| )} | |
| </> |
| if (response?.latitude && response?.longitude) { | ||
| setCountryCode(response.country_code ?? null); | ||
| setInitialCoordinates({ | ||
| latitude: response.latitude, | ||
| longitude: response.longitude, |
There was a problem hiding this comment.
The IP geolocation success check uses truthiness (if (response?.latitude && response?.longitude)), which will incorrectly treat valid coordinates of 0 as missing (e.g., locations on the equator/prime meridian). Use explicit number checks (e.g., typeof === "number" and Number.isFinite) instead.
| if (response?.latitude && response?.longitude) { | |
| setCountryCode(response.country_code ?? null); | |
| setInitialCoordinates({ | |
| latitude: response.latitude, | |
| longitude: response.longitude, | |
| const { latitude, longitude } = response ?? {}; | |
| const hasValidCoordinates = | |
| typeof latitude === "number" && | |
| Number.isFinite(latitude) && | |
| typeof longitude === "number" && | |
| Number.isFinite(longitude); | |
| if (hasValidCoordinates) { | |
| setCountryCode(response.country_code ?? null); | |
| setInitialCoordinates({ | |
| latitude, | |
| longitude, |
| // Keep state aligned with the URL. Only fetch when the slug has actually | ||
| // changed from what we resolved locally. We intentionally do *not* clear | ||
| // `selectedListing` when the slug goes away — the drawer animates out and | ||
| // should keep showing the last listing until it's fully closed — but we | ||
| // do clear the pin-selection id so the pin snaps back immediately. | ||
| useEffect(() => { | ||
| if (!listingSlug) { | ||
| resolvedSlugRef.current = null; | ||
| setOptimisticListingId(null); | ||
| return; | ||
| } | ||
|
|
||
| // On first mount: if SSR gave us the listing for this slug, use that. | ||
| if ( | ||
| listingSlug === initialListingSlug && | ||
| initialListing && | ||
| resolvedSlugRef.current !== listingSlug | ||
| ) { | ||
| setSelectedListing(initialListing); | ||
| resolvedSlugRef.current = listingSlug; | ||
| setOptimisticListingId(initialListing.id ?? null); | ||
| return; | ||
| } | ||
|
|
||
| if (resolvedSlugRef.current === listingSlug) { | ||
| return; | ||
| } | ||
|
|
||
| fetchBySlug(listingSlug); | ||
| }, [fetchBySlug, initialListing, initialListingSlug, listingSlug]); |
There was a problem hiding this comment.
The URL-sync effect skips fetching when resolvedSlugRef.current === listingSlug, but tableName (public vs private view) can change when user becomes available/unavailable. In that case the effect will keep showing the previously fetched view and never refetch the listing with the correct columns. Consider also tracking the resolved table/view (or resetting resolvedSlugRef when tableName changes) so the listing refetches on auth state changes.
| type Debounced<T extends (...args: never[]) => unknown> = T & { | ||
| cancel: () => void; | ||
| }; | ||
|
|
||
| // Trailing-edge debounce. Keeps the hook dependency-light (no @types/lodash). | ||
| function debounce<T extends (...args: never[]) => unknown>( |
There was a problem hiding this comment.
The debounce utility is typed with T extends (...args: never[]) => unknown, which suggests the wrapped function takes no arguments and is easy to trip over (and makes the types harder to understand). This helper should accept arbitrary parameters (e.g. unknown[]/any[]) so the typings match usage like debouncedFetch(bounds).
| type Debounced<T extends (...args: never[]) => unknown> = T & { | |
| cancel: () => void; | |
| }; | |
| // Trailing-edge debounce. Keeps the hook dependency-light (no @types/lodash). | |
| function debounce<T extends (...args: never[]) => unknown>( | |
| type Debounced<T extends (...args: any[]) => unknown> = T & { | |
| cancel: () => void; | |
| }; | |
| // Trailing-edge debounce. Keeps the hook dependency-light (no @types/lodash). | |
| function debounce<T extends (...args: any[]) => unknown>( |
- Always render Map so DEFAULT_COORDINATES applies when IP lookup is null - Accept latitude/longitude 0 in MapTiler geolocation response - Refetch listing when public/private view changes after auth resolves - Loosen debounce helper typings (document why any[] is needed) Co-authored-by: Danny White <dnywh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/components/ListingChatDrawer/ListingChatDrawer.tsx:98
listingDisplayNameis still part ofListingChatDrawerPropsbut is no longer used (it’s renamed to_listingDisplayName). This creates dead API surface and can confuse callers. Either remove the prop from the component + call sites, or reintroduce its usage where intended (e.g. in the CTA/button label or ChatWindow header).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => reject(new Error("Location timeout")), 3000); | ||
| }); | ||
|
|
||
| const response = (await Promise.race([ | ||
| geolocation.info(), | ||
| timeoutPromise, | ||
| ])) as { |
There was a problem hiding this comment.
The timeout implemented via timeoutPromise can still reject after geolocation.info() wins the Promise.race, which can trigger an unhandled promise rejection in the browser. Track the timeout id and clearTimeout once the race settles (or wrap the timeout promise so its rejection is always handled), and consider clearing it in the effect cleanup as well.
| const fetchBySlug = useCallback( | ||
| async (slug: string) => { | ||
| try { | ||
| const { data, error } = await supabase | ||
| .from(tableName) | ||
| .select() | ||
| .eq("slug", slug) | ||
| .single(); | ||
|
|
||
| if (error) { | ||
| setSelectedListing({ | ||
| error: true, | ||
| message: t("Listings.edit.notFound"), | ||
| }); | ||
| setOptimisticListingId(null); | ||
| return; | ||
| } | ||
|
|
||
| const listing = data as Listing; | ||
| setSelectedListing(listing); | ||
| resolvedSlugRef.current = slug; | ||
| resolvedTableRef.current = tableName; | ||
| setOptimisticListingId(listing.id ?? null); | ||
| } catch (err) { |
There was a problem hiding this comment.
fetchBySlug can race with subsequent URL changes (or view flips): an earlier request resolving late will still call setSelectedListing and overwrite the newer selection. Add stale-request protection (e.g., a requestIdRef/token captured per call and checked before setting state, or AbortController if supported) so only the latest slug/tableName result is applied.
| const selectListingById = useCallback( | ||
| async (id: number) => { | ||
| // Tap → pin grows immediately, even before the network round-trip. | ||
| setOptimisticListingId(id); | ||
|
|
||
| try { | ||
| const { data, error } = await supabase | ||
| .from(tableName) | ||
| .select() | ||
| .eq("id", id) | ||
| .single(); | ||
|
|
||
| if (error || !data) { | ||
| setSelectedListing({ | ||
| error: true, | ||
| message: t("Listings.edit.notFound"), | ||
| }); | ||
| setOptimisticListingId(null); | ||
| return; | ||
| } | ||
|
|
||
| const listing = data as Listing; | ||
| setSelectedListing(listing); | ||
| const slug = listing.slug; | ||
|
|
||
| if (slug) { | ||
| // Claim this slug so the URL sync effect doesn't refetch. | ||
| resolvedSlugRef.current = slug; | ||
| resolvedTableRef.current = tableName; | ||
| router.push(`/map?listing=${slug}`, { scroll: false }); | ||
| } |
There was a problem hiding this comment.
selectListingById has the same out-of-order response risk: if a user taps multiple pins quickly, a slower first request can resolve after the later one and overwrite selectedListing / URL with the wrong listing. Introduce a per-selection request token (or AbortController) and ignore results that are no longer current before calling setSelectedListing / router.push.
| const handleDrawerOpenChange = useCallback(() => { | ||
| // Drawer-driven close (e.g. escape key on desktop) should also update | ||
| // the URL. | ||
| if (isListingSelected) { | ||
| closeListing(); | ||
| } | ||
| }, [closeListing, isListingSelected]); |
There was a problem hiding this comment.
onOpenChange provides the next open state, but handleDrawerOpenChange ignores the argument and always calls closeListing() whenever a listing is selected. This can inadvertently close the drawer if the callback fires on open as well as close. Accept the open boolean and only call closeListing() when open === false.
| const handleDrawerOpenChange = useCallback(() => { | |
| // Drawer-driven close (e.g. escape key on desktop) should also update | |
| // the URL. | |
| if (isListingSelected) { | |
| closeListing(); | |
| } | |
| }, [closeListing, isListingSelected]); | |
| const handleDrawerOpenChange = useCallback( | |
| (open: boolean) => { | |
| // Drawer-driven close (e.g. escape key on desktop) should also update | |
| // the URL. | |
| if (!open && isListingSelected) { | |
| closeListing(); | |
| } | |
| }, | |
| [closeListing, isListingSelected] | |
| ); |
| export function getListingCoordinates( | ||
| listing: Listing | ListingMarker | SelectedListing | null | undefined | ||
| ): ListingCoordinates | null { | ||
| if (!listing) return null; | ||
| if (!isListing(listing as SelectedListing)) return null; | ||
| return (listing as Listing | ListingMarker).coordinates ?? null; | ||
| } |
There was a problem hiding this comment.
getListingCoordinates currently relies on isListing(listing as SelectedListing) plus casts, which is confusing and can mis-classify non-Listing shapes (e.g. ListingMarker) because isListing only checks error !== true. Consider switching to an explicit isListingError/'error' in listing check and narrowing without casts, or split overloads for SelectedListing vs ListingMarker to keep the type guard accurate.
- useIpInitialLocation: track setTimeout id and clear it in finally + effect cleanup so the losing race branch can't fire after the network call wins. - useMapListingUrl: add a monotonic requestTokenRef; fetchBySlug and selectListingById ignore stale responses (and close invalidates any in-flight fetch) so older requests can't overwrite newer selections. - MapPageClient: handleDrawerOpenChange now accepts the open arg and only calls closeListing when the drawer is actually closing. - mapUtils: narrow getListingCoordinates / hasValidCoordinates via an explicit isListingError check instead of casting through isListing. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/components/ListingChatDrawer/ListingChatDrawer.tsx:98
listingDisplayNameis still a required prop onListingChatDrawerPropsbut it’s not used (it’s renamed to_listingDisplayName). This creates unnecessary prop-drilling and can confuse callers. Either use it (e.g., in the CTA button label) or remove it from the props and update call sites accordingly.
src/features/map/components/MapSidebar.tsx:176- The
steps.maplist uses the translatedstep.titlestring as the React key. Because this text can change (e.g., locale switch) and could theoretically collide, it’s safer to use a stable identifier (e.g., add anid: "find" | "contact" | "dropOff"field and key off that).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (!isDesktop || !isListingSelected) return; | ||
|
|
||
| const handleScroll = () => { | ||
| if (drawerContentRef.current) { | ||
| setIsDrawerHeaderShown( | ||
| drawerContentRef.current.scrollTop > SCROLL_THRESHOLD | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const observer = new MutationObserver(() => { | ||
| const drawerContent = drawerContentRef.current; | ||
| if (drawerContent) { | ||
| drawerContent.addEventListener("scroll", handleScroll); | ||
| observer.disconnect(); | ||
| } | ||
| }); | ||
|
|
||
| observer.observe(document.body, { childList: true, subtree: true }); |
There was a problem hiding this comment.
In the desktop scroll-listener effect, the handler is only attached inside the MutationObserver callback. If drawerContentRef.current is already set when the effect runs (likely, since refs are assigned before effects), no DOM mutation may occur and the scroll listener never attaches, so the sticky header state won’t update on desktop. Attach the listener immediately when drawerContentRef.current is non-null, and only fall back to MutationObserver when it’s still null.
Previously, the desktop branch of useMapDrawerState only attached the scroll listener inside the MutationObserver callback. If drawerContentRef.current was already populated when the effect ran (common on subsequent listing changes), no DOM mutation fired, the listener never attached, and the sticky drawer header stayed hidden. Now we attach immediately when the ref is set and only fall back to the observer when it isn't. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/components/ListingRead/ListingRead.tsx:56
createClient()is called during render, sosupabasegets a new identity every render. Becausesupabaseis included in theuseEffectdependency list, this can cause the “load existing thread” effect to re-run repeatedly (potentially an infinite fetch/setState loop). Memoize the Supabase client (e.g., create it once viauseMemo/useRef) or move creation out of render, then update the effect deps accordingly so it only runs when the relevant inputs change (user/listing).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (error || !data) { | ||
| setSelectedListing({ | ||
| error: true, | ||
| message: t("Listings.edit.notFound"), | ||
| }); | ||
| setOptimisticListingId(null); | ||
| return; |
There was a problem hiding this comment.
On fetch failure in selectListingById, the hook sets selectedListing to an error sentinel but does not update the URL. Since the drawer open/close state is URL-driven, this error state may never be shown (when no listing is in the URL), or it can momentarily show an error while the URL still points at the previous listing (state/URL mismatch). Consider keeping selectedListing unchanged on failure (and just clear the optimistic id), or explicitly push a URL state that matches the error/closed state so UI + URL stay consistent.
| return { | ||
| south: Math.max(-90, sw.lat - latPad), | ||
| north: Math.min(90, ne.lat + latPad), | ||
| west: sw.lng - lngPad, | ||
| east: ne.lng + lngPad, | ||
| }; |
There was a problem hiding this comment.
padBounds expands west/east longitudes without normalizing them into a valid range or handling antimeridian wrapping. With renderWorldCopies enabled, LngLatBounds can be “unwrapped”, and padding increases the chance of west/east falling outside [-180, 180] (or producing a box that crosses the dateline), which can break the listings_in_view RPC bounding-box query. Consider wrapping/clamping longitudes (and splitting into two bboxes when crossing the antimeridian) before issuing the request.
…padBounds - selectListingById no longer sets the error sentinel when the fetch fails. Tap-driven fetches happen before the URL is pushed, so the drawer would either stay closed (swallowing the error) or desync UI and URL while pointing at the previous listing. The optimistic pin id is reverted and `selectedListing` is left untouched, so UI and URL stay consistent. - padBounds now wraps longitudes into [-180, 180] and splits the envelope into two when the padded viewport crosses the antimeridian. Callers iterate all returned boxes. - useListingsInView fetches all returned boxes in parallel and merges responses, deduping by id. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/components/ListingRead/ListingRead.tsx:56
createClient()returns a new Supabase client instance on every render (seeutils/supabase/client.ts), and this component’s thread-loading effect depends onsupabase. That means the effect will rerun (and refetch the thread) on any re-render (e.g. toggling the chat drawer). Memoize the client withuseMemo(or hoist it) so the instance is stable and the effect only reruns when listing/user changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (error || !data) { | ||
| // Tap-driven fetches happen before the URL is pushed, so surfacing | ||
| // an error sentinel here would either be invisible (no listing in | ||
| // URL → drawer stays closed) or desync the UI from the URL (still | ||
| // pointing at the previous listing). Revert the optimistic pin and | ||
| // leave `selectedListing` alone instead. | ||
| console.warn("Failed to select listing by id:", error); | ||
| setOptimisticListingId(null); | ||
| return; |
There was a problem hiding this comment.
In selectListingById, when the fetch-by-id fails you clear optimisticListingId to null while leaving selectedListing and the URL pointing at the previously selected listing. This can leave the drawer showing listing A but with no pin highlighted (because selectedListingId is derived only from optimisticListingId). Consider restoring the optimistic id back to the current resolved listing id (or tracking the previous selected id in a ref) on failure instead of setting it to null.
When selectListingById's fetch fails while a previously resolved listing is still visible in the drawer, the pin selection was being cleared to null — leaving the drawer showing listing A with no pin highlighted. Now we capture the currently-resolved listing id (via a ref mirroring selectedListing.id) before the optimistic change, and revert to that id on failure so the pin and the drawer stay in sync. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
src/components/ListingChatDrawer/ListingChatDrawer.tsx:103
isNestedis declared optional, butshouldUseModaltreats onlyisNested === falseas the non-nested case. If a caller ever omitsisNested, desktop will fall into the non-modal path, which contradicts the comment and may change behavior unexpectedly. Consider defaultingisNestedtofalsein the function params (or using a condition that treatsundefinedthe same asfalse).
src/components/ListingChatDrawer/ListingChatDrawer.tsx:98listingDisplayNameis still part ofListingChatDrawerProps, but it isn’t used (it’s immediately renamed to_listingDisplayName). If it’s no longer needed, remove it from the props type and from the call sites to avoid dead props; otherwise, use it somewhere visible so it stays consistent withListingRead.
src/components/ListingRead/ListingRead.tsx:56createClient()returns a new Supabase client on every render. Sincesupabaseis later included in the dependency array for theloadExistingThreadeffect, this can cause the effect to re-run after each state update and repeatedly refetch. Consider memoizing the client (e.g. viauseMemo/useRef) so its reference is stable across renders.
src/components/ListingRead/ListingRead.tsx:51mapZoomLevelstate (and the effect that sets it) is currently unused —MapThumbnailusesinitialZoomLeveldirectly. Consider removing the unused state/effect to avoid dead code and potential lint warnings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (!listingSlug) { | ||
| resolvedSlugRef.current = null; | ||
| resolvedTableRef.current = null; | ||
| setOptimisticListingId(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the URL loses listingSlug (e.g. user hits browser Back while a fetchBySlug request is in-flight), the in-flight request token is not invalidated. A late response can still pass the token check and set selectedListing/optimisticListingId, re-selecting a pin even though the URL has no listing. Consider incrementing requestTokenRef.current (same as closeListing) when listingSlug becomes null to cancel any outstanding fetches.
- useMapListingUrl: bump requestTokenRef when listingSlug becomes null so a late fetchBySlug response can't re-select the listing after browser back. - ListingRead: memoize the Supabase client so the thread-loading effect doesn't re-run on every render; drop unused mapZoomLevel state + effect (MapThumbnail already uses initialZoomLevel directly). - ListingChatDrawer: drop the dead listingDisplayName prop (was renamed to a discarded local); simplify isNested check to !isNested so an omitted prop is treated as "not nested". Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/ListingRead/ListingRead.tsx:99
- The thread-loading effect’s dependency array includes the entire
realListingobject, which will cause the effect (and Supabase query) to rerun whenever the listing object identity changes, even if the ids are the same. Consider removingrealListingfrom the deps and instead depending on the specific fields used inside the effect (e.g.realListing?.id,realListing?.owner_id) plususer?.id,supabase, andisDemo.
src/components/MapPin/MapPin.tsx:7 ListingPinTypeduplicates the listing-type union that already exists insrc/types/listing.ts. To avoid the two drifting out of sync, consider reusing the shared type (e.g.type ListingPinType = ListingTypeor importingListingTypeand using it directly) for the icon map / type guard.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ListingRead: the thread-loading effect depended on the whole realListing object, so any parent re-render with a new listing identity (even with the same id/owner_id) refetched the thread. Depend on the specific fields the effect reads instead. - MapPin: drop the duplicated ListingPinType union and key the icon map on the shared ListingType. Adding a new listing type now surfaces as a compile error in MapPin, keeping the two from drifting out of sync. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/components/MapPin/MapPin.tsx:158
isListingPinTypeuses theinoperator, which returns true for inherited keys (e.g."toString" in iconMap). Because callers can pass arbitrary strings, this can incorrectly narrow unknown values toListingTypeand lead toiconMap[type]being undefined. Prefer an own-property check (e.g.Object.hasOwn(iconMap, value)orhasOwnProperty.call) before narrowing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const t = useTranslations(); | ||
| const router = useRouter(); | ||
| const searchParams = useSearchParams(); | ||
| const supabase = createClient(); | ||
|
|
There was a problem hiding this comment.
createClient() creates a new Supabase browser client on every render (see src/utils/supabase/client.ts). Because supabase is a dependency of fetchBySlug/selectListingById, this causes unnecessary callback churn and can trigger extra effect work. Memoize the client with useMemo (or hoist it into a stable ref) so the same instance is reused across renders.
| export const DEFAULT_COORDINATES: ListingCoordinates & { zoom: number } = { | ||
| latitude: -27.4683, | ||
| longitude: 153.0322, | ||
| zoom: 9, | ||
| }; |
There was a problem hiding this comment.
DEFAULT_COORDINATES includes a zoom property but current callers (e.g. MapView.resolveInitialViewState) never use it, and instead fall back to ZOOM_LEVEL_DEFAULT. This is internally inconsistent and makes it easy for the fallback zoom to drift from the documented default. Either remove zoom from DEFAULT_COORDINATES or use it as the ultimate zoom fallback to keep the source of truth unambiguous.
- MapView: restore the hasInitialPosition gate. MapLibre's initialViewState is consumed once at mount, so mounting before the IP lookup resolves locks the map to DEFAULT_COORDINATES (Brisbane) even after IP data arrives. The gate waits for either a selected listing or the IP fallback to resolve before mounting. - useIpInitialLocation: on timeout or error, fall back to DEFAULT_COORDINATES so the gate always eventually lifts (addresses the earlier concern about the map never rendering on IP failure). - mapUtils: drop the unused DEFAULT_COORDINATES.zoom; ZOOM_LEVEL_DEFAULT is already the canonical default zoom. - useMapListingUrl: memoize the Supabase client so fetchBySlug / selectListingById don't churn on every render. - MapPin: tighten isListingPinType with hasOwnProperty.call so inherited prototype keys like "toString" can't incorrectly narrow to ListingType. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [countryCode, setCountryCode] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (skip) return; |
There was a problem hiding this comment.
When skip is true, this effect returns without ever setting initialCoordinates. Downstream, MapView currently gates rendering the map on Boolean(initialCoordinates) unless the selected listing has valid coordinates. This combination can prevent the map from mounting for deep links that resolve to an error sentinel or to a listing with coordinates: null. Consider setting initialCoordinates to the default fallback immediately when skip is true (or otherwise guaranteeing a non-null value), while still avoiding the IP geolocation request.
| if (skip) return; | |
| if (skip) { | |
| setInitialCoordinates({ | |
| ...DEFAULT_COORDINATES, | |
| zoom: ZOOM_LEVEL_DEFAULT, | |
| }); | |
| return; | |
| } |
| // for either a selected listing or the IP-based (or fallback) initial | ||
| // centre to resolve before mounting the Map. `useIpInitialLocation` | ||
| // always resolves (to DEFAULT_COORDINATES on failure), so this cannot | ||
| // stall indefinitely. | ||
| const hasInitialPosition = | ||
| hasValidCoordinates(selectedListing) || Boolean(initialCoordinates); |
There was a problem hiding this comment.
hasInitialPosition only becomes true when the selected listing has valid coordinates or initialCoordinates is set. When the page is deep-linked (skip: true in useIpInitialLocation) and the slug is invalid (or the listing has coordinates: null), initialCoordinates never resolves and hasValidCoordinates(selectedListing) is false, so the <Map> never mounts and the page renders an empty map container. Consider allowing the map to mount with DEFAULT_COORDINATES (or ensuring initialCoordinates is always set even when skip is true), so deep links to missing/coordinate-less listings still show the map + error drawer instead of a blank screen.
| // for either a selected listing or the IP-based (or fallback) initial | |
| // centre to resolve before mounting the Map. `useIpInitialLocation` | |
| // always resolves (to DEFAULT_COORDINATES on failure), so this cannot | |
| // stall indefinitely. | |
| const hasInitialPosition = | |
| hasValidCoordinates(selectedListing) || Boolean(initialCoordinates); | |
| // for either a selected listing, the IP-based initial centre, or the | |
| // built-in DEFAULT_COORDINATES fallback before mounting the Map. This | |
| // ensures deep links to invalid or coordinate-less listings still render | |
| // the map and related error UI instead of stalling behind an empty | |
| // container when the IP lookup is skipped or unresolved. | |
| const hasInitialPosition = | |
| hasValidCoordinates(selectedListing) || | |
| Boolean(initialCoordinates) || | |
| Boolean(DEFAULT_COORDINATES); |
Deep links (skip: true) would leave initialCoordinates null forever; combined with a listing that has no coordinates or resolves to an error sentinel, the <Map> never mounted. Fall back to DEFAULT_COORDINATES on skip so MapView can always render. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/ListingRead/ListingRead.tsx:250
- These external links open in a new tab via
target="_blank"but do not setrel="noopener noreferrer", which can allow reverse-tabnabbing. Add an appropriaterelwhen using target blank (or update the Button/Link abstraction to default it).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Holistic refactor of the map page. Fixes a few production issues and restructures the spaghetti into a focused feature folder.
Behaviour
console.logs removed.Architecture
src/features/map/withcomponents/,hooks/,lib/, and anindex.tsbarrel.MapImmersive→MapView;MapListingDrawer→MapListingDrawerPanel.MapViewnow ownsmapRef,useListingsInView, anduseMapCenter.useMapDrawerScrollreplaced byuseMapDrawerStatethat also owns snap state, html-class toggling, and listing-change reset.useMapCenter.flyToCoordinate— one owner for all programmatic map motion.config.apiKeymoved out of module scope into a guarded init insideuseIpInitialLocation.MAP_STYLEis now a module-level constant;SIDEBAR_WIDTHandSNAP_POINTSdeduplicated intolib/mapUtils.ts.Listing/SelectedListing/ListingMarkertypes insrc/types/listing.ts.ListingReadandListingChatDrawerconverted to TSX; chat drawer state moved local toListingRead.map/page.js→page.tsxwith typedgenerateMetadataandPage.Two new i18n keys:
Map.returnToListing,Map.loadingPins.npx tsc --noEmitnpm run checknpm run build